Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for DDJ-200 4-deck mode #237

Merged
merged 11 commits into from
Nov 1, 2020

Conversation

fkbreitl
Copy link
Contributor

No description provided.

source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
fkbreitl and others added 2 commits October 23, 2020 14:43
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
fkbreitl and others added 3 commits October 24, 2020 21:42
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned table

source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is easier to grasp.

source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the column alignment in the second table.

Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

columns aligned

@Holzhaus
Copy link
Member

Thanks. There are some pre-commit issues due to whitespace. Check the pre-commit check for the diff. Also, I think you need to git pull upstream manual-2.3.x to fix the rstcheck issues.

@fkbreitl
Copy link
Contributor Author

But the pre-commit errors don't seem related to my changes but to a Hercules controller.
What rstcheck?
I haven't cloned the repo locally.

@Holzhaus
Copy link
Member

But the pre-commit errors don't seem related to my changes but to a Hercules controller.
What rstcheck?

One of the pre-commit hooks that checks if the RST markup is valid.

I haven't cloned the repo locally.

Please do an pull the lastest manual-2.3.x branch from upstream into this branch.

@fkbreitl
Copy link
Contributor Author

fkbreitl commented Oct 30, 2020 via email

@Holzhaus
Copy link
Member

Sounds complicated. What do you want me to achieve?

It actually isn't. Just open a shell and run the following commands:

git clone git@github.com:fkbreitl/manual.git mixxx-manual
cd mixxx-manual
git remote add upstream https://github.com/mixxxdj/manual.git
git checkout pioneer-ddj-200
git pull upstream manual-2.3.x

Then commit the merge and run:

git push origin pioneer-ddj-200

@fkbreitl
Copy link
Contributor Author

Ok, I did it. There are no pre-commit issues.
Why was git pull upstream manual-2.3.x necessary?
Did I fork from the wrong branch or repo?

@Holzhaus
Copy link
Member

No, there was a issue present in the upstream branch when you forked. It was fixed upstream in the meantime, but the fix was not part of this branch. By pulling in the changes, we can now review this properly and know that this branch is pre-commit warning-free.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this ready to merge. One minor issue below.

source/hardware/controllers/pioneer_ddj_200.rst Outdated Show resolved Hide resolved
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@Holzhaus Holzhaus merged commit e57473f into mixxxdj:manual-2.3.x Nov 1, 2020
@fkbreitl
Copy link
Contributor Author

fkbreitl commented Nov 8, 2020

Does it make sense to delete this repo and fork it from mixxxdj/manual in case of new contributions?

@Holzhaus
Copy link
Member

Holzhaus commented Nov 8, 2020

No, you can keep it. If you want to make a new contribution, I suggest to use the upstream remote as basis in any case:

If you don't have it (you can check with git remote -v):

git remote add upstream https://github.com/mixxxdj/manual.git

Next time you want to contribute, you can just do:

git fetch upstream
git checkout -b mybranch upstream/2.3

@fkbreitl
Copy link
Contributor Author

fkbreitl commented Nov 8, 2020

Alright. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants